Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue 15750: PIP-105: Store Subscription properties #15757

Merged
merged 5 commits into from
Jun 3, 2022
Merged

Issue 15750: PIP-105: Store Subscription properties #15757

merged 5 commits into from
Jun 3, 2022

Conversation

eolivelli
Copy link
Contributor

Fixes #15750

Motivation

The Subscription properties are not properly stored on MetadataService

Modifications

Store the subscription properties on the MetadataService (ManagedCursorInfo)

Verifying this change

This change is already covered by existing tests, I have only added some additional assertions

@eolivelli eolivelli added type/bug The PR fixed a bug or issue reported a bug release/2.10.1 labels May 24, 2022
@eolivelli eolivelli added this to the 2.11.0 milestone May 24, 2022
@eolivelli eolivelli self-assigned this May 24, 2022
@github-actions
Copy link

@eolivelli:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@eolivelli eolivelli requested a review from lhotari May 24, 2022 12:06
@eolivelli eolivelli added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels May 24, 2022
@eolivelli
Copy link
Contributor Author

Even if we are changing some .proto file I believe that we can cherry-pick this change to branch-2.10, in order to unblock PIP-105. This feature wasn't working, so we are not going to break anything.

@eolivelli eolivelli force-pushed the fix/pip-105-store-properties branch from 9d125d1 to c7be86a Compare May 24, 2022 12:31
codelipenghui
codelipenghui reviewed May 24, 2022
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do the consumers change the subscription properties?
The change looks like the properties will not change after the cursor is created.

The previous behavior is the subscription will apply the properties from the first connected consumer.

dlg99
dlg99 approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@dlg99 dlg99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, couple of nitpicks.

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Show resolved Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java Show resolved Hide resolved
@eolivelli
Copy link
Contributor Author

eolivelli commented May 24, 2022 •
edited
Loading

How do the consumers change the subscription properties? The change looks like the properties will not change after the cursor is created.

The previous behavior is the subscription will apply the properties from the first connected consumer.

You are right.
I have a follow up patch in which I add support for updating the cursorProperties.
I will port part of those changes here in order to persist the properties populated from the first Consumer that sets a non empty property set.

Honestly I don't like that much that behavior and I would prefer to let the user modify the properties using an explicit PulsarAdmin function (see #15751 )

@codelipenghui do you know why we allowed to override the properties from the first Consumer?

@eolivelli
Copy link
Contributor Author

@codelipenghui I have updated this patch.
Now the behaviour is 100% backward compatible

eolivelli
eolivelli commented May 25, 2022
View reviewed changes
Copy link
Contributor Author

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codelipenghui I have updated the patch.
@dlg99 At the moment I have used the same utilities used in the files, I believe that we should decide the overall style for the things you suggested and apply the changes globally

@eolivelli eolivelli requested a review from lhotari May 25, 2022 07:27
nicoloboschi
nicoloboschi requested changes May 25, 2022
View reviewed changes
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Outdated Show resolved Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Outdated Show resolved Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Show resolved Hide resolved
.../src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentSubscription.java Outdated Show resolved Hide resolved
...broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java Outdated Show resolved Hide resolved
@codelipenghui
Copy link
Contributor

Honestly I don't like that much that behavior and I would prefer to let the user modify the properties using an explicit PulsarAdmin function (see #15751 )

@codelipenghui do you know why we allowed to override the properties from the first Consumer?

Hmm, I'm thinking more about it. The correct behavior should not be the first consumer override the properties, it should be "if the subscription absents, the first consumer will trigger the subscription creation, so the subscription will create with the properties that the first consumer has". The initial position also follows this way.

So it should be the initial implementation of PIP-105 missed to persist the properties, causing the "first consumer override properties"

@eolivelli
Copy link
Contributor Author

@nicoloboschi @codelipenghui I have addressed your comments, PTAL

@codelipenghui I think it is better to not change the behaviour for this patch

nicoloboschi
nicoloboschi approved these changes May 25, 2022
View reviewed changes
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eolivelli eolivelli force-pushed the fix/pip-105-store-properties branch from 514cbc5 to 2e06583 Compare May 25, 2022 10:29
@eolivelli
Copy link
Contributor Author

@codelipenghui , as discussed offline, I have changed the behavior of subscription properties, now they are like "initial position". You can set them only at creation time.
In my follow up patch #15751 I am adding support for updating them using PulsarAdmin

@eolivelli
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli eolivelli force-pushed the fix/pip-105-store-properties branch from b200b02 to 72430a7 Compare May 25, 2022 14:53
@eolivelli eolivelli mentioned this pull request May 26, 2022
Merged
@eolivelli
Copy link
Contributor Author

/pulsar-bot rerun-failure-checks

@eolivelli
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

lhotari
lhotari reviewed May 26, 2022
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a question about thread safety.

...broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java Show resolved Hide resolved
@mattisonchao mattisonchao mentioned this pull request May 26, 2022
Closed
@eolivelli
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli eolivelli force-pushed the fix/pip-105-store-properties branch from 7c056d7 to 203726c Compare May 27, 2022 12:38
@eolivelli
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli eolivelli force-pushed the fix/pip-105-store-properties branch from 203726c to f354f0d Compare May 31, 2022 11:13
@eolivelli
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@eolivelli eolivelli force-pushed the fix/pip-105-store-properties branch from f354f0d to e6a67d4 Compare June 1, 2022 06:37
@nicoloboschi
Copy link
Contributor

/pulsarbot rerun-failure-checks

lhotari
lhotari approved these changes Jun 1, 2022
View reviewed changes
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Good work @eolivelli

@eolivelli
Copy link
Contributor Author

/pulsar-bot rerun-failure-checks

1 similar comment
@eolivelli
Copy link
Contributor Author

/pulsar-bot rerun-failure-checks

@lhotari lhotari merged commit 23f46a0 into apache:master Jun 3, 2022
nicoloboschi pushed a commit that referenced this pull request Jun 3, 2022
@eolivelli @nicoloboschi
Issue 15750: PIP-105: Store Subscription properties (#15757)
b46924c 
* PIP-105: Store Subscription properties

(cherry picked from commit 23f46a0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlg99 dlg99 dlg99 approved these changes

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@315157973 315157973 315157973 approved these changes

@lhotari lhotari lhotari approved these changes

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@michaeljmarshall michaeljmarshall Awaiting requested review from michaeljmarshall

Assignees

@eolivelli eolivelli

Labels
area/broker cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.10.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.

PIP-105 Per-subscription properties are not persistent to MetadataStore
6 participants
@eolivelli @codelipenghui @nicoloboschi @lhotari @dlg99 @315157973